Hi @ihrpr ,

Sorry for the direct tag. I really appreciate that this is already on the HPR list, and I completely understand you have a lot on your plate.

This is just a gentle nudge on the PR - a decision here would really help me move forward on my side, especially if the change is accepted.

Thanks again for all the amazing work on the SDK! It’s been a real pleasure working with it over the past three months ;-)

One additional detail worth mentioning: the OAuth implementation used by my company (and others as well 😉) includes JWT tokens in the final authentication response. These tokens encode valuable metadata such as user identity, organization context, and more.

This is yet another reason to allow client implementations to fully control the authentication flow - they may want to extract and act on this information in ways that are specific to their environment.

👋 Hello,

I just wanted to follow up on this PR. I’ve been keeping it up to date over the past month, including adapting to changes like the recent addition of protected resource support (RFC 8707), which this PR now explicitly handles.

I really appreciate that it was marked as an HPR and I took that as a sign that it might be reviewed soon. I also sent a (hopefully gentle) nudge to @ihrpr at the time, just to make sure it was on the radar.

My main reason for commenting now is to ask for a bit of clarity: I’m more than happy to continue monitoring and updating this PR for as long as there’s a reasonable chance it might be reviewed and potentially merged. I sincerely believe it improves the SDK’s OAuth2 support and brings tangible value to the community.

That said, I totally understand if this PR isn’t likely to be accepted either for technical or strategic reasons. However, in this case, I’d rather step back than keep chasing changes unnecessarily.

Regardless, thanks again for all the work you do on this project!

Hey @m-paternostro sorry for the delay here, this is on my list, just didn't get to it this week.

hey @m-paternostro thanks for the patience here. I had a long chat with some other folks (@ihrpr , @dsp-ant, @ochafik ) and we're thinking that an even higher level approach would be better here, and I wanted to see if it'd work for you and if you'd be up for adjusting this PR to tackle it.

It'd look something like this:

  // Simple auth wrapper type
  type FetchWrapper = (fetch: FetchLike) => FetchLike;

  // The default implementation
  const withOAuth = (provider: OAuthProvider): FetchWrapper =>
    (fetch) => async (input, init) => {
      const headers = new Headers(init?.headers);
      const tokens = await provider.tokens();
      if (tokens) {
        headers.set('Authorization', `Bearer ${tokens.access_token}`);
      }

      const response = await fetch(input, { ...init, headers });
      if (response.status === 401) {
          const resourceMetadataUrl = extractResourceMetadataUrl(response);

          const result = await auth(provider, { serverUrl: this._url, resourceMetadataUrl, fetchFn: fetch });
          if (result !== "AUTHORIZED") {
            throw new UnauthorizedError();
          }

          const tokens = await provider.tokens();
          if (tokens) {
            headers.set('Authorization', `Bearer ${tokens.access_token}`);
          }

          return await fetch(input, { ...init, headers });
      }

      return response;
    };


  // Adding a custom wrapper:
  const transport = new StreamableHttpClientTransport(url, {
    fetch: withCustomOAuth(oauthProvider)(withLogging(fetch))
  });

The idea is then we could support any type of custom auth there, or other request/response munging.

Let me know what you think.

Hey @pcarleton - sorry for the delay (was out on PTO as mentioned 😊)

I really like the direction this design is heading. I took a pass at implementing the default OAuth and logging wrappers you suggested. And as you were likely signaling, the logic now feels simple and flexible enough that writing custom wrappers is straightforward - no need for a dedicated CustomAuth type or withCustomOAuth.

Btw, for now I kept everything in a single file under shared - we should probably split it, placing the withOAuth in client.

Let me know your thoughts!

@m-paternostro Thank you. This is really good and nearly where we want it.

I took the liberty to refactor this and call it middleware and move it to client/ to avoid confusion with middleware in servers. I think this fits the common terminology better. Let me know if that works for you.

There is still a lint error around usage of console, which i believe existed in your version. I can take a look but if you get to it before, i'd much appreciate it. I think we are very close to accepting this.

@dsp-ant Great news!

Your changes look good! Some comments:

• Do we need the deprecated types FetchWrapper and FetchMiddleware? I don't see other references to them on the project.
• Should applyMiddleware be applyMiddlewares? You know the naming conventions more than I do.
• The createMiddleware is very nice!

Regarding the lint errors, the offending code is related to providing logging for fetch operations, so probably not related to stdio. Would the use of console.log and console.error be OK or is there a better alternative? Btw, the error does not show up on shared where the code was initially located - the no console rule only applies to the files in client and server.

@dsp-ant I just pushed a commit with changes related to my questions on the previous comment.

Please take a look specially on the usage of console.log and console.error - they are used by the default logger, which is automatically selected if the code invoking withLogging does not provider a logger.

@pcarleton and @dsp-ant I've updated both the title and description of this PR to reflect the focus on providing a middleware programming model.

this looks great! I'd like to try sticking it into the transports to replace the existing stuff before merging. the actual replacement can be a different PR but just want to make sure it works

this looks great!

Team work ;-)

just want to make sure it works

The unit tests are inspired by the authentication tests for the transports so I'd like to believe the basics are covered and, kudos to the design, the code is simple enough to pass the "eyes inspection" check. But I definitely don't want to claim this is fully tested, accounting for all OAuth nuances.

I'd like to try sticking it into the transports

The src/examples/client/simpleOAuthClient.ts is not working for me - I get an error when I run node dist/cjs/examples/client/simpleOAuthClient.js - and, unfortunately, I don't have time this week to debug the issue.

I am bringing this up because that example is probably a good way to test the changes here... We could use the withOAuth instead of passing the oauthProvider directly to the StreamableHTTPClientTransport on line 187.

ahh great, thanks I'll start there. once i get that working, I want to see take a stab at what we'd rm from the transport files (e.g. i think things like _authThenStart will go away)

Let me know if there is anything I can do to help to have this merged. And even after that ;-)